Conversation
|
This pull request introduces 1 alert when merging 76443e0 into 5d7c04f - view on LGTM.com new alerts:
|
ErikBjare
force-pushed
the
dev/config-toml
branch
from
76443e0 to
f19f7e0
Compare
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
- Coverage 90.84% 90.75% -0.09%
==========================================
Files 34 34
Lines 1616 1655 +39
Branches 247 254 +7
==========================================
+ Hits 1468 1502 +34
- Misses 115 118 +3
- Partials 33 35 +2
Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging f19f7e0 into b2d9179 - view on LGTM.com new alerts:
|
ErikBjare
force-pushed
the
dev/config-toml
branch
from
f19f7e0 to
2325b35
Compare
|
This pull request introduces 1 alert when merging 2325b35 into b2d9179 - view on LGTM.com new alerts:
|
ErikBjare
force-pushed
the
dev/config-toml
branch
from
2325b35 to
f4b1bb8
Compare
|
This pull request introduces 1 alert when merging f4b1bb8 into b2d9179 - view on LGTM.com new alerts:
|
aw_core/config.py
Outdated
| config = f.read() | ||
| config_toml = tomlkit.parse(config) | ||
| else: | ||
| # TODO: If file doesn't exist, write with commented-out default config |
There was a problem hiding this comment.
This is already implemented below, right?
There was a problem hiding this comment.
Ah yes, forgot to remove the TODO
|
|
||
| def save_config_toml(appname: str, config: str) -> None: | ||
| # Check that passed config string is valid toml | ||
| assert tomlkit.parse(config) |
There was a problem hiding this comment.
Should we have a schema to test against here and/or in the tests?
Also does that assert result in a good runtime error?
There was a problem hiding this comment.
Since each user (server, watchers) of the API decides itself what config options to use I'm not sure if a schema makes sense.
But maybe it could check against the default config or something to make sure things are of the same type etc? Sounds like a lot of work though, and it will probably work just fine and errors should be rare and clearly user-caused (since the user will know that they edited the config).
There was a problem hiding this comment.
Would be nice with a schema, but considering the work needed I don't think it's worth it.
I don't think the configs are large enough as of now that making a schema would add much value, if we find in the future that we would like that we can add that.
|
Looks good to me. Were you planning on implementing any sort of automatic migration from the old config to this new one? |
|
Not really, automatic migrations sounds like a lot of work. I'm interested in getting this right though so if you have any suggestions for how the API could be improved or made more reliable please let me know. Right now the save_config takes a str input, while the load_config returns a special dict (part of tomlkit) that maintains formatting. |
|
Now that I think about it, it would be good if we use a similar config API as in aw-server-rust, so might be worth comparing before merging. How does this look to you @johan-bjareholt? |
johan-bjareholt
left a comment
johan-bjareholt
left a comment
There was a problem hiding this comment.
Do we need this toml support for something?
The built-in python .ini file was fine for our use-cases so I don't see the need for this, but I might be out of the loop on something.
Now that I think about it, it would be good if we use a similar config API as in aw-server-rust, so might be worth comparing before merging.
How the files are written is the same, commented with a "#" on each line to show the default.
Regarding programming API there's no point in comparing for two reasons:
- it's very hard to make the API similar as the rust toml library has very strict parsing while this does not, it's not really comparable
- The aw-server config.rs is not really written as a library so the API is just whatever fits aw-server-rust use-cases right now
| else: | ||
| # TODO: If file doesn't exist, write with commented-out default config | ||
| with open(config_file_path, "w") as f: | ||
| f.write(_comment_out_toml(default_config)) |
There was a problem hiding this comment.
This is the same way that aw-server-rust does it.
Whether it's the best solution though I'm not sure as the default config might become outdated if it gets updated in the future. It's a decent solution anyway. The other possible solutions I have in mind are neither better or worse just different, so might as well just be consistent with how aw-server-rust does it.
|
|
||
| def save_config_toml(appname: str, config: str) -> None: | ||
| # Check that passed config string is valid toml | ||
| assert tomlkit.parse(config) |
There was a problem hiding this comment.
Would be nice with a schema, but considering the work needed I don't think it's worth it.
I don't think the configs are large enough as of now that making a schema would add much value, if we find in the future that we would like that we can add that.
The stdlib ConfigParser doesn't allow for writing comments to the config file, and since we write the file every time we load the config we have default configs that are impossible to distinguish from overrides, which makes it impossible to update defaults. As an example, ActivityWatch/aw-watcher-window#52 adds a new config option, and the first time the user runs aw-watcher-window with this new option, it gets written to the config file, which makes it impossible to change again in a future version (without disrespecting possible over-decided overrides). That is why this PR only writes a commented out example config, which is the real motivation. |
|
This pull request introduces 1 alert when merging 043cc3c into aa14c8b - view on LGTM.com new alerts:
|
|
Merging this, needed for other stuff I'm working on. Will mean a config-reset in v0.11, but we need to do that at some point anyway, might as well get it over with. |
|
This pull request introduces 1 alert when merging 067bb69 into aa14c8b - view on LGTM.com new alerts:
|
3 participants
Note: This deprecates the old config functions and leaves it up to users of the config API to migrate to the new functions themselves. It's possible there might be a better way.